Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add release workflow #19

Merged
merged 2 commits into from
Nov 22, 2023
Merged

feat: add release workflow #19

merged 2 commits into from
Nov 22, 2023

Conversation

ericswanson-dfinity
Copy link
Member

@ericswanson-dfinity ericswanson-dfinity commented Nov 22, 2023

Description

Adds the release process workflow and documentation.

Notes:

  • prepare-release.toml stores the settings for changelog rolling so that the later cargo release --execute step does not also roll them
    • One of the places cargo release looks for configuration is $WORKSPACE/release.toml, so this sibling seems reasonable.
  • Commit 828e4ed referenced in the changelog is the first commit on the main branch

Fixes https://dfinity.atlassian.net/browse/SDK-1277

How Has This Been Tested?

I wasn't able to fork this repo (I imagine because it is private), so I made a different repo and then copied the changes here.

I had to open the PRs manually, because gh pr create failed. It's possible that some repo setting is required, or that this is enabled at the org level.

pull request create failed: GraphQL: GitHub Actions is not permitted to create or approve pull requests (createPullRequest)

@ericswanson-dfinity ericswanson-dfinity requested a review from a team as a code owner November 22, 2023 00:14
Copy link

@smallstepman smallstepman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I don't think this is the best approach. It forces the person who will run the process to install tooling, read the process documentation; running this version of the process means jumping between browser and CLI.

The same process could be more simple from the operator perspective, for example:

  1. manually run workflow that: opens PR with suggested version and changelog changes
  2. once PR is approved and/or merged, another automatically triggered workflow kicks in, which builds the binaries and uploads them to GH Releases.

Doing it ^that way ensures the easiest time for the process operator (no knowledge about/installation of tooling is necessary, and the process documentation can boil down to a single sentence), which I think should be the ultimate goal for repetitive, frequently occurring processes that don't change often, which is the case here.

Nevertheless, I'm approving, because you said you don't wish to be blocked.

Comment on lines 34 to 40
- name: Install dependencies
run: |
wget https://github.com/cargo-bins/cargo-binstall/releases/latest/download/cargo-binstall-x86_64-unknown-linux-musl.tgz
tar -xvzf cargo-binstall-x86_64-unknown-linux-musl.tgz
rm cargo-binstall-x86_64-unknown-linux-musl.tgz
mv cargo-binstall $HOME/.cargo/bin
cargo binstall cargo-release -y

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Install dependencies
run: |
wget https://github.com/cargo-bins/cargo-binstall/releases/latest/download/cargo-binstall-x86_64-unknown-linux-musl.tgz
tar -xvzf cargo-binstall-x86_64-unknown-linux-musl.tgz
rm cargo-binstall-x86_64-unknown-linux-musl.tgz
mv cargo-binstall $HOME/.cargo/bin
cargo binstall cargo-release -y
- uses: cargo-bins/cargo-binstall@main
- name: Install dependencies
run: cargo binstall cargo-release -y

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, will take care of it. I made cycles-ledger workflow before I contributed uses: cargo-bins/cargo-binstall@main to their repo

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- name: Roll changelog, bump version, and push branch
run: |
# see https://opensource.axo.dev/cargo-dist/book/workspaces/cargo-release-guide.html#using-cargo-release-with-pull-requests
cargo release "${{ inputs.level }}" --execute --no-confirm --config prepare-release.toml

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a benefit of having a separate .toml file for release config over storing the configuration in root Cargo.toml?

I also think that release-prepare.toml (vs prepare-release.toml) is easier to find and mentally parse when eyeballing files in the repo, and conveys same msg

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit is that cargo release doesn't roll the changelog

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. The --config points to a file that has changelog rolling configured.

Copy link

@smallstepman smallstepman Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you mean calling cargo release --execute as last step of the process (as mentioned in contributing.md) has a different behavior. If that's the case, then I find it confusing or at least, not very obvious

Copy link
Member Author

@ericswanson-dfinity ericswanson-dfinity Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the GH workflow runs cargo release ... --config prepare-release.yml, it needs to roll the changelog, so that that goes in the PR.

After the PR is merged, the cargo release --execute final step should not roll the changelog (which would create an additional commit).

Comment on lines 77 to 84
echo "PR created by this workflow: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" >> BODY.md
echo "After merging, run the following:" >> BODY.md
echo "'''bash" >> BODY.md
echo "git checkout main" >> BODY.md
echo "git pull" >> BODY.md
echo "cargo dist plan" >> BODY.md
echo "cargo release --execute" >> BODY.md
echo "'''" >> BODY.md

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using heredoc would improve readability

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I didn't know that was possible in a yaml file.

prepare-release:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- uses: actions/checkout@v3
- uses: actions/checkout@v4

Comment on lines 30 to 32

- name: Install Rust
uses: dtolnay/rust-toolchain@stable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Install Rust
uses: dtolnay/rust-toolchain@stable

@ericswanson-dfinity ericswanson-dfinity merged commit 325e132 into main Nov 22, 2023
12 checks passed
@ericswanson-dfinity ericswanson-dfinity deleted the release-workflow branch November 22, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants